feat: use data-service sessions endpoint #3296

andre-code · 2024-09-02T15:16:06Z

PR to use data-service /sessions/* endpoint in Renku V2.

What's Included

Launch sessions with a custom or global environment
Open sessions in the session view
See the current session state
Start, resume, hibernate, and shut down a session

What's Not Included ‼️

Automatic injection of saved cloud storage secrets
Displaying an error message when a session has an error

Pending

Update e2e tests

/deploy renku=feat-jupyter-free-sessions renku-data-services=release-amaltheas-migration amalthea-sessions=main renku-notebooks=master renku-gateway=1.1.0 extra-values=amalthea-sessions.deployCrd=false

RenkuBot · 2024-09-02T15:18:29Z

You can access the deployment of this PR at https://renku-ci-ui-3296.dev.renku.ch

leafty

General note: please use the function style for components.

A first batch of comments below:

leafty · 2024-10-16T08:25:24Z

client/src/components/LogsV2.tsx

+interface EnvironmentLogsPropsV2 {
+  name: string;
+}
+export const EnvironmentLogsV2 = ({ name }: EnvironmentLogsPropsV2) => {


function style -> https://github.com/SwissDataScienceCenter/renku-ui/blob/main/CODING_GUIDELINES.md#r004-include-a-default-export-when-appropriate

leafty · 2024-10-16T08:25:28Z

client/src/components/LogsV2.tsx

+  if (!logs?.show || logs?.show !== name || !logs) return null;
+
+  return (
+    <Modal
+      isOpen={!!logs.show}
+      className="modal-xl"
+      scrollable={true}
+      toggle={() => {
+        toggleLogs(name);
+      }}
+    >
+      <ModalHeader
+        className="header-multiline"
+        toggle={() => {
+          toggleLogs(name);
+        }}
+      >
+        <div>Logs</div>
+      </ModalHeader>
+      <ModalBody>
+        <div className="mx-2">
+          <SessionLogs fetchLogs={fetchLogs} logs={logs} name={name} />
+        </div>
+      </ModalBody>
+    </Modal>
+  );


Why is this split into another component here? This code can be in the previous component and that saves all the interface and const = lines of code.

leafty · 2024-10-16T08:27:46Z

client/src/features/admin/AddSessionEnvironmentButton.tsx

@@ -86,13 +86,13 @@ function AddSessionEnvironmentModal({
        addSessionEnvironment({
          container_image: data.container_image,
          name: data.name,
-          default_url: data.default_url.trim() ? data.default_url : undefined,
-          description: data.description.trim() ? data.description : undefined,
+          default_url: data.default_url?.trim() ? data.default_url : undefined,


For here and everywhere else: should we instead have this?

Suggested change

default_url: data.default_url?.trim() ? data.default_url : undefined,

default_url: data.default_url?.trim() || undefined,

Otherwise we send untrimmed strings to the API.

This can be added to a new issue to do later.

leafty · 2024-10-16T08:46:35Z

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx

+        {session?.status?.will_hibernate_at &&
+          session?.status?.will_hibernate_at?.length > 0 && (


No need for a length check on strings:

Suggested change

{session?.status?.will_hibernate_at &&

session?.status?.will_hibernate_at?.length > 0 && (

{session?.status?.will_hibernate_at && (

leafty · 2024-10-16T08:49:58Z

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx

+              Please note that paused session are deleted after{" "}
+              {hibernationThreshold} of inactivity.


This is not an accurate text copy because session?.status?.will_hibernate_at refers to this specific session. Also the text refers to "DELETE" while the threshold is for "PAUSE".

leafty · 2024-10-16T08:51:33Z

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx

+  const hibernationThreshold = session?.status?.will_hibernate_at
+    ? toHumanRelativeDuration({
+        datetime: session?.status?.will_hibernate_at,
+        now,
+      })
+    : 0;


To make things correct, it is preferable to use <TimeCaption> instead of computing this. <TimeCaption> supports passing class names and the noCaption attribute to not have the time-caption class.

leafty · 2024-10-16T08:54:50Z

client/src/features/sessionsV2/session.utils.ts

+    throw new Error("Unsupported protocol");
+  } catch (error) {
+    if (url && !url.includes("://")) {
+      return `https://${url}`;


What if this is not a valid URL?

leafty · 2024-10-16T08:57:48Z

client/src/features/sessionsV2/SessionShowPage/SessionIframe.tsx

+  session: SessionV2;
+}
+
+export default function SessionIframe({


New issue: re-factor into existing <SessionJupyter> component.

leafty · 2024-10-16T08:58:17Z

client/src/features/sessionsV2/SessionShowPage/ShowSessionPage.tsx

@@ -88,12 +85,15 @@ export default function ShowSessionPage() {
    slug: slug ?? "",
  });

-  const { data: sessions, isLoading } = useGetSessionsQuery();
+  const { data: sessions, isLoading } = useGetSessionsQuery(undefined, {
+    skip: false,


ciyer

Looks very good! I have some small code suggestions and a question about intent, but those are all quite minor.

We discussed the use of anonymous vs. named functions for the components, and it looks like the project session test is failing, but I'm assuming you are already work on those.

ciyer · 2024-10-16T08:34:34Z

client/src/features/admin/AddSessionEnvironmentButton.tsx

+          default_url: data.default_url?.trim() ? data.default_url : undefined,
+          description: data.description?.trim() ? data.description : undefined,


I would this code should actually be this:

Suggested change

default_url: data.default_url?.trim() ? data.default_url : undefined,

description: data.description?.trim() ? data.description : undefined,

default_url: data.default_url?.trim() ? data.default_url.trim() : undefined,

description: data.description?.trim() ? data.description.trim() : undefined,

or maybe even better, introduce a function:

function trimmedOrUndefined(value: string | undefined) { return value?.trim() ? value.trim() : undefined; }

ciyer · 2024-10-16T08:38:26Z

client/src/features/admin/UpdateSessionEnvironmentButton.tsx

@@ -105,13 +105,13 @@ function UpdateSessionEnvironmentModal({
          environmentId: environment.id,
          container_image: data.container_image,
          name: data.name,
-          default_url: data.default_url.trim() ? data.default_url : "",
-          description: data.description.trim() ? data.description : "",
+          default_url: data.default_url?.trim() ? data.default_url : "",


Similar to the comment on AddSessionEnvironmentButton:

function trimmedOrEmpty(value: string | undefined) { return value?.trim() ? value.trim() : ""; }

ciyer · 2024-10-16T08:53:07Z

client/src/features/session/components/StartSessionProgressBar.tsx

+    statusData?.ready_containers && statusData?.total_containers
+      ? `${statusData?.ready_containers} of ${statusData?.total_containers} containers ready`


This should be safe:

Suggested change

statusData?.ready_containers && statusData?.total_containers

? `${statusData?.ready_containers} of ${statusData?.total_containers} containers ready`

statusData?.ready_containers && statusData?.total_containers

? `${statusData.ready_containers} of ${statusData.total_containers} containers ready`

ciyer · 2024-10-16T08:55:31Z

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx

+              Please note that paused session are deleted after{" "}
+              {hibernationThreshold} of inactivity.


Suggested change

Please note that paused session are deleted after{" "}

{hibernationThreshold} of inactivity.

Please note that paused sessions are deleted after{" "}

{hibernationThreshold} of inactivity.

ciyer · 2024-10-16T09:32:18Z

client/src/features/sessionsV2/components/SessionButton/ActiveSessionButton.tsx

@@ -577,12 +556,12 @@ function ModifySessionModalContent({
  useEffect(() => {
    const currentSessionClass = resourcePools
      ?.flatMap((pool) => pool.classes)
-      .find((c) => c.id === currentSessionClassId);
+      .find((c) => `${c.id}` == resource_class_id);


I think this would be more idiomatic.

Suggested change

.find((c) => `${c.id}` == resource_class_id);

.find((c) => ""+ c.id == resource_class_id);

This is a javascript "hack" and should not be used. It uses type coercion so this makes code look weird (see things like "1"+1 == 11, etc. ${c.id} is a valid way to convert a number to a string.

ciyer · 2024-10-16T09:33:45Z

client/src/features/sessionsV2/components/SessionButton/ActiveSessionButton.tsx

+              (resource_class_id != null &&
+                resource_class_id === `${currentSessionClass?.id}`)


Since this logic is used in a couple of places:

Suggested change

(resource_class_id != null &&

resource_class_id === `${currentSessionClass?.id}`)

resourceClassMatchesCurrent(resource_class_id, currentSessionClass)

function resourceClassMatchesCurrent(resourceClassId: string | undefined, currentSessionClass: string) { return (resourceClassId != null && resourceClassId === ""+currentSessionClass.id) }

why not just?

resource_class_id === `${currentSessionClass?.id}

This is all that's needed here (resource_class_id cannot be null-ish and equal a string).

ciyer · 2024-10-16T09:37:49Z

client/src/features/sessionsV2/components/SessionButton/ActiveSessionButton.tsx

+            (resource_class_id != null &&
+              resource_class_id === `${currentSessionClass?.id}`)


Suggested change

(resource_class_id != null &&

resource_class_id === `${currentSessionClass?.id}`)

resourceClassMatchesCurrent(resource_class_id, currentSessionClass)

ciyer · 2024-10-16T09:51:19Z

client/src/features/sessionsV2/sessionsV2.api.ts

+      providesTags: (result) =>
+        result
+          ? [
+              ...result.map(({ name }) => ({


Question: name is used here, but id below. Is that correct?

The name field is the identifier. Maybe that should be changed in the API?

Yes, the change has been requested. I will implement this in a future PR once the updates are made in the API.

- use named functions - refactor working and mount directory handling with optional chaining and fallback to undefined - remove forced URL generation in ensureHTTPS function when the input is not a valid URL. - ensure session data is fetched when loading ShowSessionPage

leafty

Should be good to go after these comments.

leafty · 2024-10-18T08:02:58Z

client/src/features/admin/UpdateSessionEnvironmentButton.tsx

@@ -105,15 +105,11 @@ function UpdateSessionEnvironmentModal({
          environmentId: environment.id,
          container_image: data.container_image,
          name: data.name,
-          default_url: data.default_url.trim() ? data.default_url : "",
-          description: data.description.trim() ? data.description : "",
+          default_url: data.default_url?.trim() ? data.default_url : "",


It seems this can be done here as well:

Suggested change

default_url: data.default_url?.trim() ? data.default_url : "",

default_url: data.default_url?.trim() || "",

leafty · 2024-10-18T08:03:55Z

client/src/features/dashboardV2/DashboardV2Sessions.tsx

-import { NotebooksHelper } from "../../notebooks";
-import { NotebookAnnotations } from "../../notebooks/components/session.types";
+import { useGetSessionsQuery as useGetSessionsQueryV2 } from "../../features/sessionsV2/sessionsV2.api";
+import "../../notebooks/Notebooks.css";


Can you keep the CSS imports as the last ones? They semantically different from the other ones, so we should make them stand out. Also keep the associated comment.

- add file comment - move css import ant the end of the import list - simplify default_url and description assignment to simplify conditional check

leafty · 2024-10-18T08:50:14Z

client/src/features/dashboardV2/DashboardV2Sessions.tsx


-// Required for logs formatting


Can you keep this comment here? Otherwise there is no indication that removing the .css import would break something.

- Restore the CSS import comment to prevent the removal of log styles

RenkuBot · 2024-10-18T14:04:30Z

Tearing down the temporary RenkuLab deplyoment for this PR.

* feat!: support custom env variables in Renku 2.0 (#3227) * feat: use data-service sessions endpoint (#3296) * feat: show an error if the docker image identifier is invalid (#3369) BREAKING CHANGES: requires renku-data-services >= v0.26.0

andre-code temporarily deployed to renku-ci-ui-3296 September 2, 2024 15:17 — with GitHub Actions Inactive

andre-code changed the base branch from main to andrea/custom-launcher-jupyter-free September 5, 2024 14:58

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from a1a5479 to 816e188 Compare September 5, 2024 14:59

andre-code temporarily deployed to renku-ci-ui-3296 September 5, 2024 15:01 — with GitHub Actions Inactive

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from 816e188 to 1086ab6 Compare September 6, 2024 12:13

andre-code temporarily deployed to renku-ci-ui-3296 September 6, 2024 12:14 — with GitHub Actions Inactive

andre-code had a problem deploying to renku-ci-ui-3296 September 9, 2024 08:10 — with GitHub Actions Failure

andre-code temporarily deployed to renku-ci-ui-3296 September 9, 2024 08:47 — with GitHub Actions Inactive

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from 1086ab6 to fbc0695 Compare September 9, 2024 09:33

andre-code temporarily deployed to renku-ci-ui-3296 September 9, 2024 09:33 — with GitHub Actions Inactive

andre-code had a problem deploying to renku-ci-ui-3296 September 13, 2024 13:01 — with GitHub Actions Failure

olevski temporarily deployed to renku-ci-ui-3296 September 16, 2024 12:12 — with GitHub Actions Inactive

olevski temporarily deployed to renku-ci-ui-3296 September 17, 2024 08:21 — with GitHub Actions Inactive

andre-code force-pushed the andrea/custom-launcher-jupyter-free branch from a03efa6 to 0df603e Compare September 19, 2024 08:48

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from 283294d to deaa61a Compare September 19, 2024 09:14

andre-code temporarily deployed to renku-ci-ui-3296 September 19, 2024 09:15 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ui-3296 September 19, 2024 13:45 — with GitHub Actions Inactive

andre-code force-pushed the andrea/custom-launcher-jupyter-free branch from 0df603e to 26364df Compare September 20, 2024 07:55

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from deaa61a to dbe673e Compare September 20, 2024 08:17

andre-code temporarily deployed to renku-ci-ui-3296 September 20, 2024 08:18 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ui-3296 September 20, 2024 09:36 — with GitHub Actions Inactive

andre-code force-pushed the andrea/custom-launcher-jupyter-free branch 3 times, most recently from f059742 to 26d6197 Compare September 24, 2024 06:28

andre-code mentioned this pull request Sep 24, 2024

feat!: support custom env variables in Renku 2.0 #3227

Merged

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from 0522b87 to 57e03e7 Compare September 24, 2024 14:29

andre-code temporarily deployed to renku-ci-ui-3296 September 24, 2024 14:30 — with GitHub Actions Inactive

andre-code added ShapeUp backend changes labels Sep 26, 2024

andre-code temporarily deployed to renku-ci-ui-3296 October 15, 2024 14:14 — with GitHub Actions Inactive

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from edb9a9d to 113e0cb Compare October 16, 2024 06:52

andre-code temporarily deployed to renku-ci-ui-3296 October 16, 2024 06:52 — with GitHub Actions Inactive

leafty reviewed Oct 16, 2024

View reviewed changes

ciyer requested changes Oct 16, 2024

View reviewed changes

andre-code mentioned this pull request Oct 17, 2024

re-factor <SessionIframe/> into existing <SessionJupyter/> component #3366

Open

andre-code had a problem deploying to renku-ci-ui-3296 October 17, 2024 13:05 — with GitHub Actions Failure

andre-code force-pushed the andrea/jupyter-free-build branch from 04c6d02 to 5c9eb97 Compare October 17, 2024 13:15

andre-code added 3 commits October 17, 2024 22:15

feat: use data service sessions api

251630a

fix: update e2e tests

70836e9

andre-code force-pushed the andrea/use-data-service-sessions-2.0 branch from bcf8906 to be32e65 Compare October 17, 2024 22:16

andre-code had a problem deploying to renku-ci-ui-3296 October 17, 2024 22:16 — with GitHub Actions Failure

andre-code temporarily deployed to renku-ci-ui-3296 October 18, 2024 07:17 — with GitHub Actions Inactive

andre-code requested review from ciyer and leafty October 18, 2024 07:53

leafty reviewed Oct 18, 2024

View reviewed changes

add code suggestions

07165fb

- add file comment - move css import ant the end of the import list - simplify default_url and description assignment to simplify conditional check

andre-code temporarily deployed to renku-ci-ui-3296 October 18, 2024 08:41 — with GitHub Actions Inactive

leafty reviewed Oct 18, 2024

View reviewed changes

add code suggestions

1fea7ba

- Restore the CSS import comment to prevent the removal of log styles

andre-code deployed to renku-ci-ui-3296 October 18, 2024 09:18 — with GitHub Actions View deployment

leafty approved these changes Oct 18, 2024

View reviewed changes

andre-code merged commit 563fdda into andrea/jupyter-free-build Oct 18, 2024
11 of 19 checks passed

andre-code deleted the andrea/use-data-service-sessions-2.0 branch October 18, 2024 14:03

andre-code mentioned this pull request Oct 18, 2024

feat: add session Jupyter free images - Renku 2.0 #3341

Merged

5 tasks

andre-code added a commit that referenced this pull request Oct 28, 2024

feat: use data-service sessions endpoint (#3296)

fd62a0e

andre-code added a commit that referenced this pull request Nov 11, 2024

feat: use data-service sessions endpoint (#3296)

7668093

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use data-service sessions endpoint #3296

feat: use data-service sessions endpoint #3296

andre-code commented Sep 2, 2024 •

edited

Loading

RenkuBot commented Sep 2, 2024

leafty left a comment

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

leafty Oct 16, 2024

andre-code Oct 17, 2024

leafty Oct 16, 2024

ciyer left a comment

ciyer Oct 16, 2024

ciyer Oct 16, 2024

ciyer Oct 16, 2024

ciyer Oct 16, 2024

ciyer Oct 16, 2024

leafty Oct 16, 2024

ciyer Oct 16, 2024

leafty Oct 16, 2024

ciyer Oct 16, 2024

ciyer Oct 16, 2024

leafty Oct 16, 2024

andre-code Oct 17, 2024

leafty left a comment

leafty Oct 18, 2024

leafty Oct 18, 2024

leafty Oct 18, 2024

RenkuBot commented Oct 18, 2024

	default_url: data.default_url?.trim() ? data.default_url : undefined,
	default_url: data.default_url?.trim() \|\| undefined,

		{session?.status?.will_hibernate_at &&
		session?.status?.will_hibernate_at?.length > 0 && (

	{session?.status?.will_hibernate_at &&
	session?.status?.will_hibernate_at?.length > 0 && (
	{session?.status?.will_hibernate_at && (

		Please note that paused session are deleted after{" "}
		{hibernationThreshold} of inactivity.

		default_url: data.default_url?.trim() ? data.default_url : undefined,
		description: data.description?.trim() ? data.description : undefined,

		statusData?.ready_containers && statusData?.total_containers
		? `${statusData?.ready_containers} of ${statusData?.total_containers} containers ready`

	.find((c) => `${c.id}` == resource_class_id);
	.find((c) => ""+ c.id == resource_class_id);

		(resource_class_id != null &&
		resource_class_id === `${currentSessionClass?.id}`)

	(resource_class_id != null &&
	resource_class_id === `${currentSessionClass?.id}`)
	resourceClassMatchesCurrent(resource_class_id, currentSessionClass)

feat: use data-service sessions endpoint #3296

feat: use data-service sessions endpoint #3296

Conversation

andre-code commented Sep 2, 2024 • edited Loading

What's Included

What's Not Included ‼️

Pending

RenkuBot commented Sep 2, 2024

leafty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leafty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenkuBot commented Oct 18, 2024

andre-code commented Sep 2, 2024 •

edited

Loading